feat(config-cli): Provide a cli for creating temporary server and dump screenshots. #2236

Wentao-Kuang · 2022-06-03T02:45:48Z

No description provided.

blacha · 2022-06-07T22:08:02Z

packages/config-cli/README.md

+Create a temporary server from a config bundle file and dump the screenshots
+
+```bash
+./bin/bmc.js screenshot-server - config s3://..../config.json.gz


I think this should just be a argument to screenshot

eg

./bin/bmc.js screenshot --config s3://.../config.json.gz

Might be good to separate to two different cli? I think this will make the cli logic not too complex.

should be pretty simiple logic ?

something like

if (this.config.value) const host = await startServer() return await screenshot(host); } return await screenshot(this.host.value)

they are both doing the same thing, screenshotting some host, so to me they are the same command

blacha · 2022-06-07T22:09:12Z

packages/config-cli/src/cli/screenshot/screenshot.server.ts

+    const port = 5000;
+    const ServerUrl = `http://localhost:${port}`;
+    const BasemapsServer = createServer(logger);
+    BasemapsServer.listen(port, '0.0.0.0', () => {


this is async you should really await for the server to start before starting chrome.

granted chrome startup is going to be way slower than the server but it could create issues if the server startup takes too long

createServer() is not async function? Do you want me to update to async?

something like this

await new Promise(resolve => BaseampsServer.listen(port, '0.0.0.0', () => resolve()))

blacha · 2022-06-07T22:09:37Z

packages/config-cli/src/cli/screenshot/screenshot.server.ts

+    Config.setConfigProvider(mem);
+
+    // Create a basemaps server.
+    const port = 5000;


this port should be a high random number, is there a npm package to find a unused port?

Using random free port from 10000 to 11000 now.

blacha · 2022-06-07T22:10:20Z

packages/config-cli/src/cli/screenshot/screenshot.ts


-  async getTileSetId(tileSetId: string, tag: string): Promise<string> {
-    if (tag === 'production') return tileSetId;
+async function getTileSetId(tileSetId: string, tag: string): Promise<string> {


I think we can remove these now.

if we are pointing at a bundled config there is not @tag to worry about any more!

Pr tag removed. We can just point a lambda function url as host to take screenshot for linz/basemaps-config.

blacha · 2022-06-13T00:35:03Z

packages/config-cli/src/cli/screenshot/screenshot.ts

+      BasemapsServer = createServer(logger);
+
+      if (BasemapsServer == null) throw new Error('Failed to Create server with the config File');
+      BasemapsServer.listen(port, '0.0.0.0', () => {


this is not waiting for this to finish loading.

Sorry, I forget this. Just fixed.

blacha · 2022-06-13T00:35:30Z

packages/config-cli/README.md

+Dump the screenshots with config file
+
+```bash
+./bin/bmc.js screenshot - config s3://..../config.json.gz


blacha · 2022-06-13T02:27:36Z

packages/config-cli/src/cli/screenshot/screenshot.ts

+    if (config != null) {
+      const port = await getPort({ port: portNumbers(10000, 11000) });
+      host = `http://localhost:${port}`;
+      const server = createServer(logger);


NIT: why create a new var if we could just use BasemapsServer

another option would be const BasemapsServer = this.startServer(config) which returns null | Server

Not sure why if I pass the BasemapsServer into the await new Promise<void>() if always return type checking errors to say that BasemapsServer could be undefined even I have checked not null before. So I create a new var to pass it after.

yeah if you did this as a function that returned Promise<Server | null> could be optimised out.

blacha · 2022-06-13T04:26:54Z

packages/config-cli/src/cli/screenshot/screenshot.ts

+
+  // Start server
+  const server = createServer(logger);
+  server.listen(port, '0.0.0.0', () => {


this lost its promise again?

Sorry, I copy from the wrong place. Just fixed.

blacha · 2022-06-13T22:50:33Z

packages/config-cli/src/cli/screenshot/screenshot.ts

+
+  // Start server
+  const server = createServer(logger);
+  await new Promise<void>((resolve) =>


return new Promise<FastifyInstance>(r => { r(server); });

blacha · 2022-06-13T22:51:20Z

packages/config-cli/src/cli/screenshot/screenshot.ts

-    if (style) return styleIdTagId;
-    return styleId;
-  }
+async function startServer(host: string, port: number, config: string, logger: LogType): Promise<FastifyInstance> {


why is host passed in just to log it?

could generate host inside the function or log outside the function

blacha · 2022-06-13T22:54:26Z

packages/config-cli/src/cli/screenshot/screenshot.ts

+
+    let BasemapsServer: FastifyInstance | undefined = undefined;
+    if (config != null) {
+      const port = await getPort({ port: portNumbers(10000, 11000) });


just use any port?

Wentao-Kuang added 2 commits June 3, 2022 14:43

Create a serve and do screenshot on it.

bceabb8

DefaultTestTiles as const

de5fdc0

Wentao-Kuang requested a review from a team as a code owner June 3, 2022 02:45

Wentao-Kuang added 2 commits June 3, 2022 15:36

Correct the references

116e900

Fix linting

8c36709

blacha reviewed Jun 7, 2022

View reviewed changes

Wentao-Kuang added 4 commits June 8, 2022 14:43

Get random port for temporary server, and remove pr tag.

dbbdc7c

Merge to master

aaf4ac5

Update basemaps config and server dependencies

e7abe26

Combine screenshot and screenshot-server as a single command.

82b67f1

Wentao-Kuang requested a review from blacha June 12, 2022 23:22

blacha reviewed Jun 13, 2022

View reviewed changes

Wentao-Kuang added 2 commits June 13, 2022 13:15

waiting the server finished before lanch chromium.

b2f5e52

Minor fix

31a0f37

Wentao-Kuang requested a review from blacha June 13, 2022 02:11

blacha reviewed Jun 13, 2022

View reviewed changes

blacha previously approved these changes Jun 13, 2022

View reviewed changes

Make an aync function to start sever.

735ab70

Wentao-Kuang dismissed blacha’s stale review via 735ab70 June 13, 2022 04:19

Wentao-Kuang requested a review from blacha June 13, 2022 04:19

blacha reviewed Jun 13, 2022

View reviewed changes

Add promise for the server lisent

0e40185

Wentao-Kuang requested a review from blacha June 13, 2022 21:14

export ConfigBundled from config package.

b05c963

blacha reviewed Jun 13, 2022

View reviewed changes

blacha approved these changes Jun 13, 2022

View reviewed changes

blacha added the automerge 🚀 label Jun 13, 2022

kodiakhq bot merged commit 0713b05 into master Jun 13, 2022

kodiakhq bot deleted the feat/screenshot-server branch June 13, 2022 22:56

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(config-cli): Provide a cli for creating temporary server and dump screenshots. #2236

feat(config-cli): Provide a cli for creating temporary server and dump screenshots. #2236

Wentao-Kuang commented Jun 3, 2022

blacha Jun 7, 2022

Wentao-Kuang Jun 8, 2022

blacha Jun 8, 2022

blacha Jun 7, 2022

Wentao-Kuang Jun 8, 2022

blacha Jun 8, 2022

blacha Jun 7, 2022

Wentao-Kuang Jun 8, 2022

blacha Jun 7, 2022

Wentao-Kuang Jun 8, 2022

blacha Jun 13, 2022

Wentao-Kuang Jun 13, 2022

blacha Jun 13, 2022

Wentao-Kuang Jun 13, 2022

blacha Jun 13, 2022

Wentao-Kuang Jun 13, 2022

blacha Jun 13, 2022 •

edited

Loading

blacha Jun 13, 2022

Wentao-Kuang Jun 13, 2022

blacha Jun 13, 2022

blacha Jun 13, 2022

blacha Jun 13, 2022

feat(config-cli): Provide a cli for creating temporary server and dump screenshots. #2236

feat(config-cli): Provide a cli for creating temporary server and dump screenshots. #2236

Conversation

Wentao-Kuang commented Jun 3, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blacha Jun 13, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

blacha Jun 13, 2022 •

edited

Loading